pkg/cli/admin/prune/images: Allow partial image retrieval #2010
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
wking
force-pushed
the
allow-partial-image-retrieval-when-pruning
branch
2 times, most recently
from
98d0b45 to
fdb30bd
Compare
When Image counts get absurdly large (hundreds of thousands), pruning can fail with errors like: Error from server (Expired): The provided continue parameter is too old to display a consistent list result. You can start a new list without the continue parameter, or use the continue token in this response to retrieve the remainder of the results. Continuing with the provided token results in an inconsistent list - objects that were created, modified, or deleted between the time the first chunk was returned and now may show up in the list. With this commit, I'm still printing that warning message, but if we got any Images back before the error, I'm still going to try and prune those, because maybe some progress in pruning will get the Image count down low enough that a future invocation will go through cleanly.
wking
force-pushed
the
allow-partial-image-retrieval-when-pruning
branch
from
fdb30bd to
6097a9b
Compare
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
WalkthroughThe PR modifies error handling in the images pruning command by introducing an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/admin/prune/images/images.go (1)
425-430: Consider refining the warning message tone.The error handling logic is sound and aligns with the PR objective to allow partial progress. However, the warning message uses informal phrasing ("we got %d, so keep going").
Consider this slightly more formal alternative:
- fmt.Fprintf(o.ErrOut, "warning: error retrieving images, but we got %d, so keep going to see if we can prune any of those: %s\n", len(allImages), err) + fmt.Fprintf(o.ErrOut, "warning: error retrieving images (%s), but %d images were retrieved successfully; attempting to prune those images\n", err, len(allImages))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/cli/admin/prune/images/images.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/admin/prune/images/images.go
🔇 Additional comments (1)
pkg/cli/admin/prune/images/images.go (1)
416-416: LGTM!The
acceptableErrorsslice declaration is appropriate for accumulating non-fatal errors during image listing.
| if errs == nil { | ||
| errs = kutilerrors.NewAggregate(acceptableErrors) | ||
| } |
There was a problem hiding this comment.
Aggregate both error sets to preserve all error context.
The current logic only returns acceptableErrors when pruning succeeds (errs == nil). If pruning fails, the acceptableErrors from image listing are lost, hiding important diagnostic information from users.
Apply this diff to aggregate both error sets:
fmt.Fprintf(o.Out, "Summary: %s\n", stats)
- if errs == nil {
- errs = kutilerrors.NewAggregate(acceptableErrors)
+ if len(acceptableErrors) > 0 {
+ allErrors := append(acceptableErrors, errs)
+ errs = kutilerrors.NewAggregate(allErrors)
}
return errsNote: kutilerrors.NewAggregate handles nil errors gracefully, so this preserves existing behavior while ensuring acceptableErrors are never lost.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if errs == nil { | |
| errs = kutilerrors.NewAggregate(acceptableErrors) | |
| } | |
| if errs == nil { | |
| errs = kutilerrors.NewAggregate(acceptableErrors) | |
| } else if len(acceptableErrors) > 0 { | |
| allErrors := make([]error, 0, len(acceptableErrors)+1) | |
| allErrors = append(allErrors, acceptableErrors...) | |
| allErrors = append(allErrors, errs) | |
| errs = kutilerrors.NewAggregate(allErrors) | |
| } |
🤖 Prompt for AI Agents
In pkg/cli/admin/prune/images/images.go around lines 537 to 539, the current
code replaces errs with only acceptableErrors when errs is nil, losing
acceptableErrors when errs is non-nil; modify this so both error sets are
aggregated into a single error (using kutilerrors.NewAggregate) regardless of
whether errs is nil, ensuring acceptableErrors are appended/combined with errs
and assigned back to errs so no error context is lost.
2 participants
When Image counts get absurdly large (hundreds of thousands), pruning can fail with errors like:
With this commit, I'm still printing that warning message, but if we got any Images back before the error, I'm still going to try and prune those, because maybe some progress in pruning will get the Image count down low enough that a future invocation will go through cleanly.